-
Notifications
You must be signed in to change notification settings - Fork 91
Deploy a SASL_PLAIN Kafka listener & test notifications against it #2300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development/2.13
Are you sure you want to change the base?
Conversation
Hello delthas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for testing SASL_PLAIN authenticated Kafka listeners with bucket notifications. It configures a new Kafka listener with SASL authentication and creates the necessary test infrastructure to verify notifications work with authenticated Kafka endpoints.
- Adds a third notification destination with SASL_PLAIN authentication support
- Configures a new Kafka listener on port 9095 with SASL authentication
- Extends test parameters and environment variables to support authenticated notifications
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ctst/world/Zenko.ts | Adds interface properties for authenticated notification destination configuration |
| tests/ctst/steps/notifications.ts | Implements test step for setting up an authenticated notification destination |
| .github/workflows/end2end.yaml | Defines environment variables for authenticated destination credentials and topic |
| .github/scripts/end2end/run-e2e-ctst.sh | Passes authenticated notification parameters to test world configuration |
| .github/scripts/end2end/configure-e2e.sh | Adds Kafka topic creation for authenticated notification destination |
| .github/scripts/end2end/configure-e2e-ctst.sh | Configures SASL_PLAIN Kafka listener and creates authenticated notification topic |
| .github/scripts/end2end/configs/notification_destinations.yaml | Defines ZenkoNotificationTarget with basic authentication credentials |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auth: basic | ||
| basic: |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The authentication type is specified as 'basic', but Kafka typically uses SASL authentication mechanisms. The field name 'basic' for the authentication credentials section (lines 41-43) may not align with Kafka's SASL_PLAIN mechanism. Verify that this authentication configuration is compatible with the SASL_PLAIN listener configured in configure-e2e-ctst.sh, or consider using more Kafka-specific terminology like 'sasl' instead of 'basic'.
| auth: basic | |
| basic: | |
| auth: sasl | |
| sasl: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- looking at ZKOP pr, this is currently
plain - should we call this field
saslinstead, as recommended by Copilot? (orsasl-plain)
47079c0 to
84be537
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
8c5d50f to
de82fb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
de82fb9 to
f2cb766
Compare
Issue: ZENKO-5106 Signed-off-by: Thomas Flament <[email protected]>
Signed-off-by: Thomas Flament <[email protected]>
f2cb766 to
a091dc4
Compare
|
Test failure appears to be unrelated: https://github.com/scality/Zenko/actions/runs/20574693679/job/59089167866#step:10:57882 |
|
Finally passed after 3 retries. |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
Signed-off-by: Thomas Flament <[email protected]>
f1789f5 to
3d0d560
Compare
SylvainSenechal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth to see what happens with the same test that you added, but unauthenticated.
given a non-authenticated notification
then i should not receive a notifciation
but maybe annoying to setup and not worth it, whatever
|
PR has conflicts ( |
| {"op": "replace", "path": "/spec/readOnlyConfig", "value": $config} | ||
| ]') | ||
| kubectl patch kafkacluster "$KAFKA_CLUSTER" --type='json' -p="$KAFKA_PATCH" | ||
| kubectl wait --for=jsonpath='{.status.state}'=ClusterRunning --timeout 10m kafkacluster "$KAFKA_CLUSTER" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
patching the existing cluster is not safe, as it is "owned" by ZKOP: which should in theory reconcile it back to the expected state... maybe it works due to some "happy" bug in ZKOP which does not notice the discrepency.
should we not just deploy another kafka cluster, to be on the safe side? (and avoid future flakiness...)
| # yq eval 'sortKeys(.)' -i deps.yaml | ||
| backbeat: | ||
| sourceRegistry: ghcr.io/scality | ||
| sourceRegistry: ghcr.io/scality/playground/delthas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be updated before merging
Issue: ZENKO-5106